feat(NODE-3517): improve index spec handling and type definitions by aditi-khare-mongoDB · Pull Request #3315 · mongodb/node-mongodb-native

aditi-khare-mongoDB · 2022-07-07T15:17:54Z

Description

parseIndexOptions() was a helper function that allowed users to multiple different input formattings for CreateIndex and normalizes them to one format.

The function was buggy, in the wrong file, and had some unused outputs, so I renamed it to getFieldHash and put it inside the makeIndexSpec function.

What is changing?

Is there new documentation needed for these changes?

Yes,

Bug Fix: need to notify users that the tuple input formatting did not work correctly for IndexDirections other than 1
Map type is now an acceptable input formatting, and it is the only one that that will preserve numerical key ordering.

What is the motivation for this change?

Bug:
the previous parseIndexOptions was buggy when a [string, IndexDirection] tuple was part of the input.

Desired Output:
{string: IndexDirection}

Current Output:
(if IndexDirection is numerical) {string: 1}
OR
(if IndexDirection is not numerical): {{string: 1} {IndexDirection: 1}

New Feature:
Can now input a Map with a single key and value, or an array of length = 1 Maps as input.
** This is important as now we can preserve numerical key ordering **

Input Examples:
ex: new Map<string, IndexDirection>([['sample_index', -1]])

ex2: [ new Map<string, IndexDirection>([['sample_index1', 1]]), new Map<string, IndexDirection>([['sample_index2', -1]]), new Map<string, IndexDirection>([['sample_index3', '2d']]) ]

Addresses:

https://jira.mongodb.org/browse/NODE-4446
assertIndexNotExists operation in unified runner fix

PR Summary:

Map as a valid input type in TS definition
Uses Map under the hood to ensure key order is preserved, esp with FLE
Tuples passed at the top level to createIndex were incorrectly parsed as string input
- createIndex(['myKey', 1]) would create { 'myKey': 1, '1': 1 }.
- Now it's correctly detected if the second arg is one of the known index directions.
- We recommend using a Map to avoid all the edge cases here.
Type strictness on this nesting of array (one or more)
Type strictness for createIndexes aligned with createIndex
- No longer accepts just Document, checks that the values are a known IndexDirection

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
Changes are covered by tests
New TODOs have a related JIRA ticket

nbbeeken · 2022-07-20T19:35:22Z

Requesting Review from: @aditi-khare-mongoDB (GH permission issues)

src/cmap/connection.ts

test/integration/client-side-encryption/driver.test.ts

test/tools/spec-runner/index.js

test/unit/operations/indexes.test.ts

src/operations/indexes.ts

baileympearson · 2022-07-25T17:43:05Z

test/integration/client-side-encryption/driver.test.ts

+    let collection: Collection;
+
+    beforeEach(async function () {
+      if (this.configuration.clientSideEncryption == null) {


What is this check for?

It is to avoid the dependency missing error that will be thrown by the newClient call that takes an autoEncryption option.

So if clientSideEncryption is nullish, we return. Is that supposed to skip these tests? If so, we should explicitly skip them and attach a skip reason.

Also, if that's the intention, should we follow the precedent we just set and use this.configuration.clientEncryption.enabled?

The tests are skipped by the metadata attached to each test

baileympearson · 2022-07-25T17:47:47Z

test/tools/unified-spec-runner/operations.ts

+  let indexes;
  try {
-    expect(await collection.indexExists(operation.arguments.indexName)).to.be.true;
+    indexes = await listIndexCursor.toArray();


Why did we change the implementation here? Is our indexExists broken?

It asserted the reverse of what the operation expects (expected indexExists to return true for the assertIndexNotExists operation.) So it needed to be flipped but also we made the code the same as the code for assertExists, listIndexes is spec-ed so seems like we should use that in the spec test runner.

test/tools/unified-spec-runner/runner.ts

baileympearson · 2022-07-25T18:13:19Z

src/operations/indexes.ts

 export type IndexSpecification = OneOrMore<
  | string
  | [string, IndexDirection]
  | { [key: string]: IndexDirection }


Removing the array options is a breaking change, because the following used to be permitted, even though it's not a valid index specification according to MongoDB:

const invalid: IndexSpecification = [[["name", 1]]]

Our precedent is to release breaking TS changes as bug fixes when applicable, which I am okay with, but maybe we should consider pulling this change (the deletion of lines 60/61) into a separate bug fix PR to not mix the bug fix in with this other work.

Of the three we're addressing here should we pull them each out and try and make this PR only about the refactor (i.e. why this bug but not the others)? (I think the tuple one might be difficult but we can see).

what "three" are you referencing here? three bugs?

Map in TS

Key order FLE

Tuple parsing issue

Type strictness on this nesting of array (one or more)

Type strictness for createIndexes aligned with createIndex

I think whether we pull these out into separate PRs/fixes or not, we should make sure that the PR description accurately reflects whatever fixes are included and that we are super clear in our release notes about the changes in behavior. It's also worth noting that the types in the documentation won't get updated on a patch, unless we manually regenerate the 4.8, which could be confusing. I think because of the scope of the changes here, this set of improvements that's coming in as a bug fix is better marked as a feat:

Oh and let's triple check to make sure that every single one of those things has full regression test coverage

Collected a summary in the description and between the type/fle/indexes.test.ts additions I see coverage for each point.

src/operations/indexes.ts

src/cmap/connection.ts

baileympearson · 2022-07-25T19:27:32Z

src/operations/indexes.ts

    this.collectionName = collectionName;

-    this.indexes = indexes;
+    // Ensure we generate the correct name if the parameter is not set


Can we move this code into a function? We discussed this a bit in the office, but I still think for readability it should be broken out. The fact that a comment is necessary to explain what this block of code is doing strongly indicates that this should be broken out into a function.

test/unit/operations/indexes.test.ts

src/operations/indexes.ts

test/integration/client-side-encryption/driver.test.ts

test/tools/spec-runner/index.js

test/tools/unified-spec-runner/match.ts

test/types/community/createIndex.test-d.ts

test/unit/operations/indexes.test.ts

baileympearson

requesting changes for visibility

… on expected

test/integration/client-side-encryption/driver.test.ts

nbbeeken marked this pull request as draft July 7, 2022 17:34

nbbeeken changed the title ~~Node 3517 improve parse index options~~ fix(NODE-3517): WIP improve parse index options Jul 7, 2022

nbbeeken added the wip label Jul 7, 2022

aditi-khare-mongoDB changed the title ~~fix(NODE-3517): WIP improve parse index options~~ fix(NODE-3517): Improve parse index options Jul 11, 2022

aditi-khare-mongoDB removed the wip label Jul 11, 2022

aditi-khare-mongoDB requested a review from nbbeeken July 11, 2022 18:04

aditi-khare-mongoDB force-pushed the NODE-3517-improve-parseIndexOptions branch from d08bfe3 to 20261ac Compare July 11, 2022 19:56

nbbeeken force-pushed the NODE-3517-improve-parseIndexOptions branch from dc35940 to 0c03cf7 Compare July 20, 2022 15:48

aditi-khare-mongoDB and others added 19 commits July 20, 2022 14:51

Added unit tests

65a67e9

refactored parseIndexOptions and put it inside makeIndexSpec

6fc9cc1

refactored parseIndexOptions and moved it inside makeIndexSpec

053c047

reorganized tests

1fa6b63

Fixed map functionality

0df78c0

wip

af386dc

lint fix

c6989f2

Removed all edits from utils.test.js

a136417

Added || 1 to tuple case

7eddd4b

remove failing test for now (cannot check map equality in JSON)

359dd57

handled incorrect map stringify singular element case

2d0dde7

fix

453679b

fixed key === 'key' conditional

91910ee

Fixed build failures hopefully

572a8c7

rebase fixup

3a017d1

wip

e9f6edb

fix: preserve key order in encryption scenarios

8bf8b11

fixups

eb78d8e

test: handle Int32s

79babb2

nbbeeken force-pushed the NODE-3517-improve-parseIndexOptions branch from 33d71dd to 79babb2 Compare July 20, 2022 18:51

nbbeeken marked this pull request as ready for review July 20, 2022 18:58

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 20, 2022

clean up fle testing

02c5a1e

aditi-khare-mongoDB commented Jul 22, 2022

View reviewed changes

chore: comments

7b3f2fd

nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 22, 2022

nbbeeken reviewed Jul 25, 2022

View reviewed changes

src/operations/indexes.ts Show resolved Hide resolved

baileympearson suggested changes Jul 25, 2022

View reviewed changes

baileympearson reviewed Jul 25, 2022

View reviewed changes

test/unit/operations/indexes.test.ts Show resolved Hide resolved

dariakp requested changes Jul 25, 2022

View reviewed changes

src/operations/indexes.ts Outdated Show resolved Hide resolved

src/operations/indexes.ts Outdated Show resolved Hide resolved

nbbeeken added 3 commits July 25, 2022 16:51

fix: address comments, streamlines naming code

4ee2651

fix: use map

4a1d509

fix: wire version below 5 check no longer needed

c770dce

nbbeeken requested a review from baileympearson July 25, 2022 21:04

dariakp requested changes Jul 25, 2022

View reviewed changes

test: small fixes

4100dbc

baileympearson suggested changes Jul 26, 2022

View reviewed changes

comments: add test for name, type test for mixed types, match asserts…

c681cba

… on expected

nbbeeken requested review from baileympearson and dariakp July 26, 2022 18:14

fix: ts for indexes

4ed0db3

dariakp reviewed Jul 26, 2022

View reviewed changes

nbbeeken added 2 commits July 27, 2022 11:56

fix: tests

92b1967

fix: lint

784c8c1

nbbeeken changed the title ~~fix(NODE-3517): Improve parse index options~~ feat(NODE-3517): improve index spec handling and type definitions Jul 27, 2022

nbbeeken requested a review from dariakp July 27, 2022 16:15

baileympearson approved these changes Jul 27, 2022

View reviewed changes

dariakp approved these changes Jul 27, 2022

View reviewed changes

baileympearson merged commit 0754bf9 into main Jul 27, 2022

baileympearson deleted the NODE-3517-improve-parseIndexOptions branch July 27, 2022 18:54

Conversation

aditi-khare-mongoDB commented Jul 7, 2022 • edited by nbbeeken Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

Uh oh!

nbbeeken commented Jul 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbbeeken Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dariakp Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aditi-khare-mongoDB commented Jul 7, 2022 •

edited by nbbeeken

Loading

nbbeeken Jul 26, 2022 •

edited

Loading

dariakp Jul 26, 2022 •

edited

Loading